Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple Asset Loop out #872

Merged
merged 8 commits into from
Jan 21, 2025
Merged

Conversation

sputn1ck
Copy link
Member

@sputn1ck sputn1ck commented Jan 9, 2025

This PR adds a simple asset loop out. Clients are able to specify a asset_id during the normal loop out operation. If an asset is provided, the loop client will try to pay the swap invoice with the connected taproot-assets daemon.

@sputn1ck sputn1ck requested a review from bhandras January 9, 2025 15:57
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 5e35488 to fd87cb9 Compare January 9, 2025 16:08
@dstadulis
Copy link
Contributor

dstadulis commented Jan 9, 2025

able to specify a asset_id during the normal loop out operation

ultimate deliverable for asset specification will need to be specification by group_key, rather that asset_id only

loopout.go Outdated Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch 4 times, most recently from 50a927f to 5b60e68 Compare January 14, 2025 17:09
@sputn1ck sputn1ck marked this pull request as ready for review January 15, 2025 16:08
@sputn1ck sputn1ck requested a review from bhandras January 15, 2025 16:16
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch 2 times, most recently from df72fdc to 8696132 Compare January 15, 2025 16:48
@sputn1ck sputn1ck requested review from starius and hieblmi January 15, 2025 19:50
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 8696132 to abf4549 Compare January 16, 2025 15:21
@lightninglabs-deploy
Copy link

@bhandras: review reminder

Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @sputn1ck, this seems pretty straight forward.

I've got mostly minor comments in my first pass. Will take a look at the int test.

loopd/view.go Show resolved Hide resolved
looprpc/client.proto Outdated Show resolved Hide resolved
loopd/swapclient_server.go Outdated Show resolved Hide resolved
interface.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
cmd/loop/loopout.go Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch 6 times, most recently from db8949c to 3315e2d Compare January 20, 2025 08:35
@sputn1ck sputn1ck requested a review from hieblmi January 20, 2025 08:35
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch 2 times, most recently from 82fa50a to 242d754 Compare January 20, 2025 16:56
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! 🔥 Looks pretty good!

assets/client.go Outdated Show resolved Hide resolved
assets/client.go Show resolved Hide resolved
assets/client.go Show resolved Hide resolved
assets/client.go Outdated Show resolved Hide resolved
assets/client.go Outdated Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
loopdb/sql_store.go Show resolved Hide resolved
loopdb/sql_store.go Outdated Show resolved Hide resolved
-- name: UpdateLoopOutAssetOffchainPayments :exec
UPDATE loopout_swaps_assets
SET
asset_amt_paid_swap = $2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an on-conflict clause?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not get into on-conflict territory, so returning an error would be okay i'd assume

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will never return an error. I meant that it could return an error if the amounts are already non-zero. It's also not super important but an extra measure that we don't update the row accidentally.

loopdb/store.go Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 242d754 to 0f54b96 Compare January 21, 2025 11:37
@sputn1ck sputn1ck requested a review from bhandras January 21, 2025 11:40
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 7bf86f8 to 90144f0 Compare January 21, 2025 11:40
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort @sputn1ck, nice additions to keep track of accounting. I've left a couple of suggestions.

assets/client.go Outdated Show resolved Hide resolved
looprpc/client.proto Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
loopdb/sqlc/queries/swaps.sql Outdated Show resolved Hide resolved
@@ -552,6 +552,9 @@ message SwapStatus {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be placed in a separate commit

loopd/swapclient_server.go Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 90144f0 to 4a47df4 Compare January 21, 2025 12:01
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐐

assets/client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
// the swap.
if request.AssetRFQRequest != nil {
rfqReq := request.AssetRFQRequest
if rfqReq.Expiry == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that expiry and limit multiplier are sane? As in they're non negative etc.

@@ -29,6 +29,7 @@ require (
github.com/lightningnetwork/lnd/clock v1.1.1
github.com/lightningnetwork/lnd/queue v1.1.1
github.com/lightningnetwork/lnd/ticker v1.1.1
github.com/lightningnetwork/lnd/tlv v1.2.6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be squashed with the commit before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AKTSCHUALLY, if you run go mod tidy in the prior commit it will remove this

loopdb/sqlc/migrations/000012_loop_out_asset_params.up.sql Outdated Show resolved Hide resolved
-- name: UpdateLoopOutAssetOffchainPayments :exec
UPDATE loopout_swaps_assets
SET
asset_amt_paid_swap = $2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will never return an error. I meant that it could return an error if the amounts are already non-zero. It's also not super important but an extra measure that we don't update the row accidentally.

assets/client.go Outdated Show resolved Hide resolved
assets/client.go Outdated Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 4a47df4 to 81130f7 Compare January 21, 2025 14:36
looprpc/client.proto Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
interface.go Outdated Show resolved Hide resolved
interface.go Outdated Show resolved Hide resolved
loopd/swapclient_server.go Outdated Show resolved Hide resolved
loopdb/loopout.go Outdated Show resolved Hide resolved
loopout.go Outdated Show resolved Hide resolved
interface.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉
Added several comments.

loopdb/sql_store.go Outdated Show resolved Hide resolved
loopdb/sql_store.go Outdated Show resolved Hide resolved
loopdb/sql_store.go Outdated Show resolved Hide resolved
loopdb/sql_store.go Outdated Show resolved Hide resolved
loopdb/store.go Outdated Show resolved Hide resolved
loopdb/store_mock.go Outdated Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch 2 times, most recently from 96a6136 to 712b659 Compare January 21, 2025 17:21
@sputn1ck sputn1ck force-pushed the asset_simple_loopout branch from 712b659 to 51bdf04 Compare January 21, 2025 17:29
@sputn1ck sputn1ck merged commit 684db85 into lightninglabs:master Jan 21, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants